-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ENH] revisions to @oesteban's Alternative proposal to BEP038 #1
Conversation
- atlas/tpl can't be used at the same time - relax requirement of cohort directory - remove language of cohort as 'dual' to session - remove references to provenance (out of scope, imo)
@@ -289,6 +289,7 @@ If you contributed to the BIDS ecosystem and your name is not listed, please add | |||
| Parul Sethi | 📖🔧⚠️💻 | | |||
| Patricia Clement | 💬🐛💻📖🔣💡📋🤔📆⚠️📢 | | |||
| Patrick Park | 📖💡💬💻 | | |||
| Paul Wighton | 📖 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pwighton To avoid losing this to a reversion, please update https://github.com/bids-standard/bids-specification/wiki/Recent-Contributors, as this page is auto-generated from a source during the release process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @effigies, I just updated that page.
src/derivatives/atlas.md
Outdated
follows the general BIDS-Derivatives pattern: | ||
|
||
```Text | ||
<pipeline_name>/ | ||
sub-<label>/ | ||
<datatype>/ | ||
<source_entities>[_space-<space>][_cohort-<label>][_atlas-<label>][seg-<label>][_scale-<label>][_res-<label>][_den-<label>][_desc-<label>]_<suffix>.<extension> | ||
<source_entities>[_space-<space>][_cohort-<label>][_atlas-<label>][_template-<label>][seg-<label>][_scale-<label>][_res-<label>][_den-<label>][_desc-<label>]_<suffix>.<extension> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a template entity should not be introduced, but rather use the tpl entity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good catch. I've reworked this so that it uses the tpl
entity, and to clarify that one can either use the tpl entity, atlas entity, but not both.
"tpl-PS13_desc-pvc_dseg.nii.gz": "", | ||
"tpl-PS13_dseg.json": "", | ||
"tpl-PS13_dseg.tsv": "", | ||
"atlas-Economo1916_desc-nopvc_dseg.nii.gz": "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to keep the tpl entity added to the filename, so the file can be shared as a standalone entity. Alternatively, make it clear that the various atlases now belong to the space PS13 (i.e. add space-PS13)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand why it can't be shared as a standalone entity as is? Regarding space
entity. I guess we should decide if space
is REQUIRED when using atlas, or if it's only REQUIRED for disambuguaiton.
src/derivatives/atlas.md
Outdated
"tpl-PS13_desc-pvc_dseg.nii.gz": "", | ||
"tpl-PS13_dseg.json": "", | ||
"tpl-PS13_dseg.tsv": "", | ||
"atals-PS13_desc-nopvc_dseg.nii.gz": "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small typo in atlas. Is it clear from this that atlas belongs to the PS13 tpl and/or PS13 space?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
"tpl-ps13_space-MNI152Lin_atlas-ps13_dseg.json": "", | ||
"tpl-ps13_space-MNI152Lin_atlas-ps13_dseg.tsv": "", | ||
"tpl-ps13_seg-fsaverage_pet.json": "", | ||
"tpl-ps13_seg-fsaverage_json.tsv": "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space-fsaverage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I've removed these entierly. They've been replaced by atlas-fsaverage_dseg.nii.gz
and atlas-fsaverage_dseg.json
"tpl-ps13_space-fsaverage_atlas-ps13_desc-pvc_dseg.nii.gz": "", | ||
"tpl-ps13_space-fsaverage_atlas-ps13_dseg.json": "", | ||
"tpl-ps13_space-fsaverage_atlas-ps13_dseg.tsv": "", | ||
"atlas-fsaverage_dseg.nii.gz": "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tpl-ps13_space-fsaverage_dseg.nii.gz?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the intent is here. This file has been lifted straight from FreeSurfer and, while used during PS13 processing (to generate results at the ROI-level) it has not been processed/modified by the PS13 pipeline.
src/derivatives/atlas.md
Outdated
"sub-01_label-head_mask.nii.gz": "", | ||
"sub-01_T1w.nii.gz": "", | ||
"sub-01_T1w.json": "", | ||
"atlas-AAL_dseg.json": "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sub-01 part should remain in the filename, to indicate that it belongs to sub-01
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Martin on this it's too easy to overwrite other subject specific files without the sub-<label>
remaining. Was this changed to make the sharing of a single subject atlas/template consistent with a multi-subject atlas/template?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted.
src/derivatives/atlas.md
Outdated
"sub-01_label-head_mask.nii.gz": "", | ||
"sub-01_T1w.nii.gz": "", | ||
"sub-01_T1w.json": "", | ||
"atlas-colin27_seg-brain_mask.nii.gz": "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sub-01 should remain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
src/derivatives/atlas.md
Outdated
"sub-01_res-1_T1w.json": "", | ||
"sub-01_res-2_T1w.nii.gz": "", | ||
"sub-01_res-2_T1w.json": "", | ||
"atlas-colin27_res-1_seg-brain_mask.nii.gz": "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is neither an atlas or template at this stage, but still sub-01. When aggregating across subjects, this will be a tpl and atlas (for segmentation).
src/derivatives/atlas.md
Outdated
"atlas-MNI152NLin2009cAsym_res-1_seg-brain_mask.nii.gz": "", | ||
"atlas-MNI152NLin2009cAsym_res-1_seg-eye_mask.nii.gz": "", | ||
"atlas-MNI152NLin2009cAsym_res-1_seg-face_mask.nii.gz": "", | ||
"atlas-MNI152NLin2009cAsym_res-1_seg-head_mask.nii.gz": "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think tpl-MNI152NLin2009cAsym_res-1_label-brain_mask.nii.gz is valid, given that the label entity is valid (https://bids-specification.readthedocs.io/en/stable/glossary.html#label-entities), which belongs to the template. I see the dseg parts as atlases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I've reverted these changes.
Please note that the [`<datatype>/` directory](../glossary.md#data_type-common_principles) is RECOMMENDED. | ||
The [`<datatype>/` directory](../glossary.md#data_type-common_principles) MAY be omitted in the case | ||
only one data type (such as `anat/`) is stored under the `tpl-<label>` directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't germane to this PR, so let me know if I should move it to the discussion at bids-standard/bids-specification.
I like the result of this, but I started trying to write some it out in the schema last night and began re-evaluating my initial thoughts. I guess it follows the same sort of logic as a session entity, so sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bendhouseart perhaps you are looking at the wrong place? This is an alternative to my alternative to the BEP. In my alternative, the schema is updated with the specs (or should be).
@pwighton This whole thing is looking really sharp and I'm having real trouble providing any suggestions that improve the language or the look of the examples. I am concerned though with being able to differentiate between single subject and group level derived files. I think Martin's comments intimate much the same. Maybe this is already handled with sources and can be left alone? There's always a balance to be struck between metadata and file names so maybe Lastly, I'm curious if @oesteban or you would object to adding in a
|
Thanks for the feedback @bendhouseart and @mnoergaard. I've updated the PR. What I think the spec needs now:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the atlas intended use is still unclear. Let's have that call and make progress.
The following entities MAY be employed to specify template- and atlas-derived results: | ||
|
||
- [`tpl-<label>`](../glossary.md#template-entities) is REQUIRED to specify derivatives defining | ||
a [template](../common-principles.md). | ||
a [template](../common-principles.md). MUST NOT be used alongside `atlas-<label>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a [template](../common-principles.md). MUST NOT be used alongside `atlas-<label>` | |
a [template](../common-principles.md). |
We have not introduced atlas- yet, so this admonition feels too preemptive
The following entities MAY be employed to specify template- and atlas-derived results: | ||
|
||
- [`tpl-<label>`](../glossary.md#template-entities) is REQUIRED to specify derivatives defining | ||
a [template](../common-principles.md). | ||
a [template](../common-principles.md). MUST NOT be used alongside `atlas-<label>` | ||
- [`atlas-<label>`](../glossary.md#atlas-entities) is REQUIRED to specify derivatives defining an [atlas](../common-principles.md). MUST NOT be used alongside `template-<label>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- [`atlas-<label>`](../glossary.md#atlas-entities) is REQUIRED to specify derivatives defining an [atlas](../common-principles.md). MUST NOT be used alongside `template-<label>` |
1- Why moving from below? It feels more clear to me the current ordering of description (1- template, 2- space, 3- cohort, 4- atlas). The reason for that is, to me, the difference between template and space is the more subtle/finicky, then cohort is necessary for template and space, and finally atlas depends on the three of them.
2- template-<label>
- you mean tpl-<label>
? If so, I don't see how atlas- can be used without tpl-.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these changes, @oesteban, and let's have a meeting soon to discuss this further. @pwighton - please correct me if I am wrong, but I think @pwighton is arguing that an atlas is currently defined as being a segmentation (defined on a template), while tpl is a an averaged feature map (i.e. template) across subjects. Therefore, it may seem unintuitive to have both tpl- and atlas- in the filename, as it cannot both be a feature map and a segmentation? The space entity would reconcile how atlas- can be used without tpl-. But if tpl- serves the same role as space- then having tpl- and atlas- in the name still makes sense, but is then an atlas and not a template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was the understanding I arrived at later (#1 (comment)).
As I argue there, the data type (whether an average feature map or a segmentation) in BIDS is expressed by the suffix. This use of atlas- is just reflecting some suffixes, and leaving tpl- for others, which is counter-productive because we have suffix that fully expresses this (and conflicting with BIDS, which doesn't change sub- depending on the suffix).
I think the best way to approach this is through the homology/analogy between subject and template and between session and cohort. If we cannot agree on that mapping, hardly we're going to find a common place for atlas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the best way to approach this is through the homology/analogy between subject and template and between session and cohort. If we cannot agree on that mapping, hardly we're going to find a common place for atlas.
I've raised this point previously, but dropped it because it didn't seem critical, but it looks like it will be. Cohort in the biological sciences generally means "a set of subjects who satisfy a set of inclusion criteria" and while this can be used to denote a session or timepoint, e.g. "subjects who have been imaged between Jan and Dec 2024", it can be used for other things as well, e.g "Subjects who were born before 1980" or "Subjects who have a BMI less than 20" or "Subjects who speak Italian".
Here are how some other controlled terminologies define cohort:
"In OHDSI research, we define a cohort as a set of persons who satisfy one or more inclusion criteria for a duration of time." 1
"A group of individuals who share a common exposure, experience or characteristic or a group of individuals followed-up or traced over time in a cohort study. [AMA Manual of Style]" 2
"Cohort Study: Observational study that assembles a large group of individuals, collects extensive information about
these people at baseline, and follows up with the group for a period of time in an attempt to examine
relationships between outcomes and risk factors." 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, the homology is scoped within BIDS:
tpl- works like sub- (and also provides a reference frame for the results within the folder tpl-/ like sub-/)
cohort- works like session- (and also provides a grouping mechanism within the corresponding folder)
beyond that, this mapping does not intend to make cohorts an equivalent to sessions.
@@ -8,45 +8,43 @@ segmentations, and other knowledge annotations such as landmarks defined with | |||
respect to individual or template brains that are supported by spaces | |||
such as surfaces and regular grids (images). | |||
|
|||
For derivatives with atlases in their provenance corresponding to individual subjects, | |||
the organization follows the standards for BIDS raw and derivatives. | |||
The organization follows the standards for BIDS raw and derivatives. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The organization follows the standards for BIDS raw and derivatives. | |
For derivatives with atlases in their provenance corresponding to individual subjects, | |
the organization follows the standards for BIDS raw and derivatives. |
How can this be removed? Perhaps what's missing is some more explicit bridge between individual subjects and group (template) specifications, but that doesn't mean that, when you are working with individual subjects you still use sub-[...] structures like the BIDS raw&derivatives specs.
BIDS-Derivatives specifications. It is also REQUIRED when [`tpl-<label>`](../glossary.md#template-entities) is used, | ||
and the template is NOT serving as the authoritative definition of a space. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for bringing this up, I feel you just made clear a blind spot in my way to think about templates and I really appreciate it :)
First as a note, please use semantic break lines following the rest of the specs, they are easier to read and review.
If I managed to understand the intent, then I think this phrasing would be more unambiguous:
BIDS-Derivatives specifications. It is also REQUIRED when [`tpl-<label>`](../glossary.md#template-entities) is used, | |
and the template is NOT serving as the authoritative definition of a space. | |
BIDS-Derivatives specifications. | |
Like for individual subject-derivatives, [`space-<label>`](../glossary.md#space-entities) | |
specifies a new authoritative coordinate system and, although data derive from | |
a particular subject, they are spatially referred to the space designated by the entity. | |
This means that the data labeled with [`space-<label>`](../glossary.md#space-entities) | |
were originally defined with the subject's scanner coordinates as the authoritative coordinate | |
system and were then *projected* or *resampled* on [`space-<label>`](../glossary.md#space-entities). | |
For example, `sub-01_space-MNI152NLin2009cAsym_T1w.nii.gz` indicates that the | |
T<sub>1</sub>-weighted (T1w) image originally defined in subject 01's scanner coordinates | |
has been resampled into `MNI152NLin2009cAsym` coordinates, what is typically | |
referred to as *a subject's T1w spatially standardized into MNI space*. | |
This naturally extends to the case of templates. | |
For example, `tpl-OASIS30ANTs_space-MNI152NLin2009cAsym_T1w.nii.gz` indicates that the | |
T<sub>1</sub>-weighted average map originally defined in `OASIS30ANTs` coordinates | |
has been resampled into `MNI152NLin2009cAsym` coordinates. |
@effigies please double check the above makes sense :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think defining where and at what time the averaging takes place is still tricky to define and make solid requirements for. For example, for PET data we usually 1) project the 4D PET to e.g. fsaverage (using anatomical data, i.e. sub-XX_space-fsaverage_pet.nii.gz), 2) smooth the data along the cortical surface, and then 3) perform kinetic modeling to produce binding estimates. We then average these binding estimates across subjects (i.e. sub becomes tpl to create a new template existing in fsaverage space). This process works well because it respects individual cortical boundaries by registering individual surfaces to fsaverage. It would instead be suboptimal to first volumetrically smooth each individual data in subject space, produce binding estimates, register all subjects to e.g. the first subject, average the binding estimates across subjects to produce a new template, and then register that template to e.g. fsaverage. But ultimately, the outcome of these two workflows would be the same i.e. average binding estimates across a set of subjects in fsaverage space. But in the first case, this might be considered an atlas on top of tpl-fsaverage (i.e. tpl-fsaverage_atlas-PET_pet.nii.gz) or a new template since we are averaging across subjects (i.e. tpl-PET_space-fsaverage_pet.nii.gz), whereas for the second case it might be considered a template in fsaverage space (i.e. tpl-PET_space-fsaverage_pet.nii.gz). Now if I define a novel PET parcellation/atlas based on this, I would have tpl-fsaverage_atlas-PET_probseg/dseg.nii.gz for case 1 (i.e. only the suffix datatype changes), and for case 2 it would be tpl-PET_space-fsaverage_atlas-PET_probseg/dseg.nii.gz.
In my humble opinion, I suggest that when averaging is taking place for a new datatype e.g. pet (instead of T1w) and a new set of subjects in either a new or already defined space, this becomes a new template existing in that defined space. A new segmentation/atlas may then subsequently be defined on top of this (with a dseg/probseg extension), and will therefore also belong to a specific template and a specific space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your example seems to me like pretty clearly belonging to tpl-fsaverage
(and your second paragraph is consistent with the changes you made in bids-standard@891bcf0, which I was planning to respond in our meeting but then got distracted and did not mention.
HOWEVER, there was a point in the meeting where we touched base on this. What we said is, considering that, even though you are working in fsaverage coordinates you are not the original originator of fsaverage, then we should recommend that a new template identifier (such as PS13) is issued.
In the dataset_description.json
then we should clearly indicate that the new template is in fact aligned with fsaverage. If that is the case, then sub-01_hemi-L_space-PS13_PET.gii
becomes absolutely equivalent to sub-01_hemi-L_space-fsaverage_PET.gii
, in both cases signifying PET data on a surface defined in fsaverage/PS13 space, which is the same.
We could (and I like the idea a lot) enforce that space is present in the filenames if the new template does not generate its own coordinate system (which is what @pwighton was basically pointing us at):
tpl-PS13/
tpl-PS13_space-MNI152Lin_T1w.nii.gz
This example tells you that you generated a T1w average (whether in a custom space of the study or you moved everything to MNI152Lin should be found in some provenance metadata and/or a paper) that is not the one distributed with MNI152Lin, but in this case it is aligned with that space.
The generation of new template identifiers can be recommended in that case, but this has effects to things like TemplateFlow more than the specification itself. BIDS can perfectly live without this recommendation, nor the combination of tpl- and space- in this use-case.
Why?
- For BIDS, the specification disallowing generating new template identifiers if they are really defined in a different, existing template space, would work as-is. It tells you how to organize the data and the organization is absolutely valid.
- TemplateFlow and other potential services care about the ownership and access rights of stakeholders. At the moment, the developers of TemplateFlow regulate that, but, if it were a "proper" registry, we should not modify say fsaverage adding PS13 when the authors are not exactly the same or the scope of fsaverage is not to cover PS13.
That said, this is already creating difficulties in understanding, so I believe BIDS should observe these nuances and I think adding these mechanisms is a good thing for the spec. But again, it's more a problem of data ownership/access/stewardship than it is of nomenclature or organization.
Happy to work through examples in the call if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this reply, @oesteban, and looking forward to work through some of the examples in our call tomorrow. Researching a bit on how already existing T1w templates were generated (https://www.lead-dbs.org/about-the-mni-spaces/) e.g. MNI152 NLIN 2009, 152 brains were nonlinearly registered into 305 space (i.e. MNI305) and then averaged to create a new template i.e. MNI152 NLIN 2009. So I think this conflicts a bit with the current notion that averaging should take place in a new space defined by the 152 brains and not in an already defined template space, as the MNI152 NLIN 2009 template would otherwise not be a new template, but instead either 1) an atlas tpl-MNI305_atlas-MNI152NLIN2009_T1.nii.gz, or 2) a new template defined in an already existing space tpl-MNI152NLIN2009_space-MNI305_T1w.nii.gz. In the PS13 case, alternatively, this should just be a new template tpl-PS13_pet.nii.gz. But adding the space entity seems necessary to differentiate between different output spaces e.g. fsaverage or MNI305?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It so happens that fsaverage IS MNI305 with an adjustment of an offset in the through-plane (Z) direction :D.
I'm aware of what you mean, however, the difference between MNI305 and MNI152NLin2009cAsym (and variants) is that the alignment into MNI305 is linear, and a new anatomical reference and registration method where employed (nonlinear, since 2006).
Because the registration is nonlinear, I would be confident to call MNI152NLin2009cAsym a new template w.r.t. MNI305.
Let's pick fsaverage, MNI305 and PS13.
PS13 is in fsaverage so long the spatial transform between PS13's maps/surfaces/etc. and fsaverage is identity. However, between MNI305 and fsaverage there is a translation and therefore the transform is not identity.
Between MNI305 and MNI152NLin2009cAsym, the linear part of the transform is identity, but the nonlinear part of the transform (which is not released by MNI because they probably did not even calculate it, I would have not calculated it) is not identity.
Now, I'm going to give you ammo to fire at my argument. Okay, the transform between MNI152NLin2009aAsym, MNI152NLin2009bAsym, MNI152NLin2009cAsym is the identity.
-> Yes, but in this case, I believe we can apply the principle explained above and allow them to be separate. Alternatively, we can change (which is not backward compatible) BIDS' existing designation of some standard spaces so that a, b, and c versions of 2009 become just MNI152NLin2009Asym.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @oesteban - I think your take on this is really helpful. For the PS13 example (and other PET examples in general), we generate the average feature map using often a volumetric stream and a surface-based stream. For the volumetric stream, we have used CVS registration (https://surfer.nmr.mgh.harvard.edu/fswiki/mri_cvs_register) estimating and applying a nonlinear registration from subject space to a CVSMNI152 template provided by FreeSurfer. After this registration, the data is averaged across subjects. To me, this aligns with https://www.lead-dbs.org/about-the-mni-spaces/ stating 'MNI152 NLIN 6th generation: Here, the 152 brains were nonlinearly registered into 305 space. Again, only a T1 template is available.', and so this volumetric part should be defined as a new template wrt. the CVSMNI152 template/space (i.e. tpl-PS13_pet.nii.gz). However, for the surface-based stream we estimate and apply the registration from subject space to fsaverage, and then average across subjects. So the spatial transform between this new surface-based feature map and fsaverage is identity, and would not be defined as a new template, and so should instead be defined as a continuous atlas with a _pet.nii.gz extension (i.e. tpl-fsaverage_atlas-PS13_pet.nii.gz'). If one would estimate a parcellation on top of this, the filename would change to tpl-fsaverage_atlas-PS13_dseg/probseg.nii.gz). Right?
I think I would be okay with this, and it would not conflict with prior templateflow work/distribution, but would impact the distribution of templates/atlases moving forward. I guess it might be slightly confusing that the same continuous PET data can be considered both a template and an atlas, and so the alternative would be to use the space entity (as you mentioned above), enforcing that space is present in the filenames if the new template does not generate its own coordinate system. Furthermore, I'd be inclined to still use the space entity in the volumetric stream case above, stating that the PS13 template now exists in CVSMNI152 space, similar to the fact that I think the tpl-MNI152 NLIN 6th generation should theoretically also have had the space-MNI305 part included. Then the PS13 data could be distributed as
tpl-PS13_space-fsaverage_pet.nii.gz
tpl-PS13_space-CVSMNI152_pet.nii.gz
tpl-PS13_space-MNI305_pet.nii.gz
tpl-PS13_space-CVSMNI152_atlas-PS13_dseg.nii.gz
- [`cohort-<label>`](../glossary.md#cohort-entities) is REQUIRED to disambiguate derivatives defined with | ||
respect to different cohort instances of a single [space (coordinate system)](../appendices/coordinate-systems.md). | ||
respect to different cohort instances. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love this deletion, but if it makes the specs clearer, I'm happy to accept.
@@ -267,7 +263,7 @@ A guide for using macros can be found at | |||
}, | |||
"tpl-MNI152NLin2009cAsym": { | |||
"anat": { | |||
"tpl-MNI152NLin2009cAsym_res-1_label-brain_mask.nii.gz": "", | |||
"atlas-MNI152NLin2009cAsym_res-1_seg-brain_mask.nii.gz": "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"atlas-MNI152NLin2009cAsym_res-1_seg-brain_mask.nii.gz": "", | |
"tpl-MNI152NLin2009cAsym_res-1_label-brain_mask.nii.gz": "", |
"sub-01_atlas-AAL_seg-brain_mask.nii.gz": "", | ||
"sub-01_atlas-AAL_seg-head_mask.nii.gz": "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides the label/seg discussion above. This example wants to exactly mean that the brain mask and the head mask WERE NOT projected from a template with those segments defined as part of the AAL atlas. Happy to add this clarification if necessary.
"sub-01_atlas-AAL_seg-brain_mask.nii.gz": "", | |
"sub-01_atlas-AAL_seg-head_mask.nii.gz": "", | |
"sub-01_label-brain_mask.nii.gz": "", | |
"sub-01_label-head_mask.nii.gz": "", |
"atlas-AAL_space-MNI305_dseg.json": "", | ||
"atlas-AAL_space-MNI305_dseg.nii.gz": "", | ||
"atlas-AAL_space-MNI305_dseg.tsv": "", | ||
"atlas-AAL_space-MNI305_probseg.nii.gz": "", | ||
"atlas-AAL_space-MNI305_seg-brain_mask.nii.gz": "", | ||
"atlas-AAL_space-MNI305_seg-head_mask.nii.gz": "", | ||
"atlas-AAL_space-colin27_dseg.json": "", | ||
"atlas-AAL_space-colin27_dseg.nii.gz": "", | ||
"atlas-AAL_space-colin27_dseg.tsv": "", | ||
"atlas-AAL_space-colin27_probseg.nii.gz": "", | ||
"atlas-AAL_space-colin27_seg-brain_mask.nii.gz": "", | ||
"atlas-AAL_space-colin27_seg-head_mask.nii.gz": "", | ||
"...", "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are losing here the notion that you are still in subject space. Definitely not a use case for atlas.
Please roll back.
@@ -14,8 +14,8 @@ atlas: | |||
is [Bijsterbosch et al. (2020)](https://doi.org/10.1038/s41593-020-00726-z). | |||
|
|||
To further differentiate the concept of *atlas* and *template* throughout the specification, | |||
*template* will designate a standardized stereotaxy frame where | |||
some feature or features are mapped through spatial normalization and then possibly averaged. | |||
*template* will designate an aggregation of ordinal data which MAY define a standardized stereotaxy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with MAY, this is not optional.
@@ -357,7 +354,10 @@ A guide for using macros can be found at | |||
|
|||
For example, the [PS13 template](https://doi.org/10.18112/openneuro.ds004401.v1.3.0), | |||
a molecular imaging brain template of Cyclooxygenase-1 (PET), | |||
was generated in two standard spaces: `MNI152Lin` and `fsaverage`: | |||
was generated in two standard spaces: `MNI152Lin` and `fsaverage`. Here, the use of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed up to this point. I will stop here and let's continue together on a call :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review, @oesteban. I think a lot of my changes come from my misunderstanding of the intent of the proposal. Looking forward to discussing and clarifying tomorrow.
@bendhouseart Indeed, this BEP could improve a lot if we get deeper into the required and optional metadata. And what you are suggesting seems very reasonable to me. At the same time, I'd be careful not to dive too deep into provenance because there's already a BEP for that, but there are some metadata definitions already in derivatives for this. Also, changing metadata can be more feasible without breaking backwards compatibility. The idea of the alternative proposal is to find a file naming structure we can agree on and then I think the metadata will come out naturally. Happy to take suggestions already though! |
Closing this PR, as I've misunderstood the intent of the proposal. I'll perform an new review and open a new PR after the holidays. |
These revisions are based off of a conversation with @oesteban and @mnoergaard in which it was clarified that an atlas is an aggregation of nominal (discreet-valued) data and a template is an aggregation of ordinal (continuous-valued) data.
The distinction between atlas template is clarified
Original proposal seemed to imply it's an altas/template only if it aggregates over more than one subject, otherwise the
sub-<label>
is used. This has been removed as it seems unnecessary and introduced complications (e.g In the colin27 example, it is not clear from the filname thatsub-01_T1w.nii.gz
is an aggregation of data across 27 scans) This should be incorporated into the definition if it is to be re-introducedRemoves the concept of 'derived from' from the definition of atlas. Provenance should be considered out of scope of this spec, imo
Removes references to "original default or implicit atlas". I don't think this concept is necessary, but if it is, it should be formally defined.
Reworked examples:
_desg
,_probseg
or_mask
it's an atlas_pet
, it's a templateatlas-ps13_res-2_space-fsaverage_desc-nopvc_dseg.tsv
in the PS13 example Is an aggregation of the data inatlas-ps13_res-2_space-fsaverage_stat-mean_desc-nopvc_mimap.nii.gz
, using the ROIs inatlas-fsaverage_dseg.nii.gz
to define how the data is aggregated. This file is now calledtpl-ps13_seg-fsaverage_pet.tsv
in this PR since it's an aggregation of continuous-valued data.